Adding Morphological Dilation and Erosion Algorithms #430
Conversation
…hem and 2 test image results
There was a problem hiding this comment.
Thanks for the interesting PR.
Here is my first quick review of it:
-
I left number of comments to clear up trivial aspects of the proposed code first, so not much of commenting on the algorithms itself.
-
Second, new features (or tweaks in behaviour of existing ones) must be covered with tests. Please, include test for the new functions you are proposing here.
-
Is it necessary to include the two PNG files?
mloskot
added
cat/feature
…ts, removed two image files added previously. TODO: Add more tests
|
Updates -
TODOs -
|
This is looking good. Thanks for your efforts to improve this. I'll get another look and review near Wednesday.
I've taken the liberty and added those as the PR tasklist which you can also edit. Another thing, you mentioned on the Boost mailing list you will be proposing those algorithms as part of your competency test. If you are working on this PR as the competency test, please update the description to make it clear this is your solution for the GSoC 2020 competency test. Since working on PR may involve much more effort than working just on C++ code of the competency test solution, you are not expected to finish-polish the PR in order to count it for a completed competency test solution. In other words, your PR does not have to be approved and merged to consider the test completed. For the competency test, the gist of C++ code is most important. Although you can postpone addressing further reviews and completion of this PR for later (e.g. during GSoC), you are very welcome to continue working on to make it ready for merging. It's your call. /cc @stefanseefeld unless he does not agree and would like to clear my explanation. |
ayushbansal07
changed the title
ayushbansal07
changed the title
I have updated the description. I'd be happy to continue working on this and want to try my best to get it to merge before submitting my GSoC Proposal :) |
There was a problem hiding this comment.
I have added a bunch of nitpicks :-)
We have a prototype of benchmarks, currently in dedicated branch, that you may find interesteing. Here you can see comparison of various pixel iteration methods available in GIL
- https://github.com/boostorg/gil/blob/benchmark/benchmark/celero/histogram_gray8.cpp
- https://github.com/boostorg/gil/blob/benchmark/benchmark/celero/charts/histogram_gray8_msvc1421_HistogramGray8.html (save it on disk and open in browser)
For me, this PR as it is now is a complete solution of your competency test and as I mentioned in #430 (comment) you are not required to make it a feature complete PR before the GSoC starts
If you still want to keep working on it, please consider add some some documentation e.g. Doxygen-like comments for erode and dilate routines.
|
@mloskot
Thank you for this interesting comparison. If you don't mind me asking, I wanted to know, just of out curiosity, what could be the reason behind such huge run time of 2d_y_iterator. Is it because each iteration would result in a cache miss? |
Yes. I recommend to watch the original video lecture about GIL presented by Lubomir Bourdev |
|
@mloskot |
There was a problem hiding this comment.
Could you also document template parameters using n D oxygen \tparam and description of type requirements or mention of concept, etc.
Optional
Unfortunately, we don't have structured prose documentation for the Image Processing features. However, perhaps you could also start scratching two Markdown files with prose for dilation and erosion, describing what's the algorithm does, how to use it, what are limitations, corner cases or not supported cases, include references or names of algorithms involved.
|
Would it be okay if I did the documentation part later (during GSoC time)? I was asking this because I was planning to implement a separate class for Structuring Element as first step of my GSOC project and some of those corner cases, etc. would depend on the design of Structuring Element class. So, I was wondering if all of that could be done together afterwards. |
Yes, certainly. That's what I also suggested in #430 (comment) I think, your PR is completed from the competency test stand point, but to get it merged it will need that extra bit of work during the GSoC. So, let's keep it open as work in progress for now. /cc @stefanseefeld |
…d resolved some nitpicks
|
@mloskot Really sorry for the late response. I was a little busy with my semester exams. I have made the required changes :) |
There is no need to be sorry. It's your PR, so you work on it at your own pace.
Please, hit the Warnings@ayushbansal07 Please, could you also check compilation output of your tests and examples and clean up any compilation warnings caused by your code? Compilation of GIL code still outputs lots of warnings and we are (slowly) clearing that, but we definitely want to avoid introduction of new warnings. |
…example) and included morphology in CMakeLists
|
@mloskot I have made relevant changes to make sure that the Cl builds and has minimal warnings. Also, FYI, I have checked Add more advanced test case(s) and Ensure all CI builds pass in the task list. |
|
@ayushbansal07 in #430 (comment)
Thank you for the very good job you've done. I consider it as more than complete for your competency test. Although the PR is very close to be approved and merged, we are not in rush My reasoning is to :
Regardless if you will participate during GSoC or not, since this PR is pretty much completed, I'm fairly sure your contribution will be merged eventually. |
mloskot
added
core
|
@mloskot Also, if it is not much of a trouble for you, please feel free to mention me in any discussion that might potentially help me understand more about GIL. Thank you again! :) |
Sure, but nothing immediately comes to mind. Feel free to browse the issues, check the good first issue list. For understanding GIL, I highly recommend getting familiar with these topics from GIL docs:
These are essential parts to understand to be comfortable with using GIL. Mind you, I'm not suggesting to memorise the API or any details of implementation, but learn vocabulary of GIL, what are the building blocks, features they offer, how to use them, when to use them. If anything needs clarifying, don't hesitate to ask questions on the boost-gil list. This should make a busy GSoC preparation for next days :) |
|
This is being replaced with more feature-complete morphological operations implementation in #541 Thanks @ayushbansal07 for your work here. |
mloskot
removed
the
status/work-in-progress
Description
Morphological Dilation and Erosion algorithms implemented (for grayscale images).
This PR is also meant to be my solution for the competency test for GSoC 2020.
Dilation/Erosion process is performed by laying the structuring element H on the image I and sliding it across the image in a manner similar to convolution.
[𝐼⊕𝐻](𝑢,𝑣)=max(𝑖,𝑗)∈𝐻{𝐼(𝑢−𝑖,𝑣−𝑗)+𝐻(𝑖,𝑗)}
[𝐼⊕𝐻](𝑢,𝑣)=min(𝑖,𝑗)∈𝐻{𝐼(𝑢−𝑖,𝑣−𝑗)+𝐻(𝑖,𝑗)}
References
Further development during GSoC 2020
Tasklist